Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

nix flake: clarify error message when file is an unknown type #12167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

nix flake: clarify error message when file is an unknown type #12167

wants to merge 1 commit into from
+59 −13

Conversation

RossComputerGuy
Copy link
Member

Motivation

Clarify the error as seen in #11217

Context

#11217 is where this behavior has been seen

Simply clarifying the error makes the user know why the failure occurred.

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Jan 9, 2025
@RossComputerGuy RossComputerGuy force-pushed the fix/unsupported-type-docker branch from 992ead7 to ad7111c Compare January 9, 2025 23:50
@RossComputerGuy RossComputerGuy mentioned this pull request Jan 9, 2025
Open
lucperkins
lucperkins suggested changes Jan 10, 2025
View reviewed changes
src/nix/flake.cc Outdated
@@ -941,7 +941,7 @@ struct CmdFlakeInitCommon : virtual Args, EvalCommand
createSymlink(target, os_string_to_string(PathViewNG { to2 }));
}
else
throw Error("file '%s' has unsupported type", from2);
throw Error("path '%s' is not either a symlink, file, or directory but a %s", from2, st.typeString());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw Error("path '%s' is not either a symlink, file, or directory but a %s", from2, st.typeString());
throw Error("path '%s' needs to be a symlink, file, or directory but instead is a %s", from2, st.typeString());

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@RossComputerGuy RossComputerGuy force-pushed the fix/unsupported-type-docker branch from ad7111c to bbf2efc Compare January 10, 2025 04:39
edolstra
edolstra approved these changes Jan 10, 2025
View reviewed changes
Copy link
Member

@edolstra edolstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I have a slight hesitation about adding non-NAR-legal types to SourceAccessor, because it's supposed to represent the NAR view of the file system. But then again, it already had a tMisc type so this doesn't really change anything.

src/libutil/git.cc
Comment on lines +321 to +325
case SourceAccessor::tChar:
case SourceAccessor::tBlock:
case SourceAccessor::tSocket:
case SourceAccessor::tFifo:
case SourceAccessor::tUnknown:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case SourceAccessor::tChar:
case SourceAccessor::tBlock:
case SourceAccessor::tSocket:
case SourceAccessor::tFifo:
case SourceAccessor::tUnknown:

We can just rely on the default case here (everything is unsupported except for the types handled above).

Copy link
Member Author

@RossComputerGuy RossComputerGuy Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get an error saying not all the cases are met when I remove it.

src/libutil/git.cc Outdated
case SourceAccessor::tBlock:
case SourceAccessor::tSocket:
case SourceAccessor::tFifo:
case SourceAccessor::tUnknown:
default:
throw Error("file '%1%' has an unsupported type", path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw Error("file '%1%' has an unsupported type", path);
throw Error("file '%1%' has an unsupported type", path);

This can use typeString() now to show the type of the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/libutil/source-accessor.cc Outdated
@@ -5,6 +5,25 @@ namespace nix {

static std::atomic<size_t> nextNumber{0};

bool SourceAccessor::Stat::isTypeUnknown() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool SourceAccessor::Stat::isTypeUnknown() {
bool SourceAccessor::Stat::isTypeUnknown()
{

Formatting nitpick.

isTypeUnknown() is a misnomer since we do know the type now. Maybe it should be something like isNotNARSerialisable() to denote that it's a type than isn't supported in a NAR file. (Or isNARSerialisable() to avoid the negation.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I did try clang-format but then it formatted every single file. It seems nix hasn't followed clang-format for some time?

@RossComputerGuy RossComputerGuy force-pushed the fix/unsupported-type-docker branch from bbf2efc to 22adffe Compare January 10, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucperkins lucperkins lucperkins requested changes

@edolstra edolstra edolstra approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

Assignees
No one assigned
Labels
new-cli Relating to the "nix" command
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RossComputerGuy @edolstra @lucperkins